Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH/BUG : Update node based geometry find element family of functions #991

Conversation

jmarquisbq
Copy link
Contributor

@jmarquisbq jmarquisbq commented Jun 13, 2024

  • properly checks for nullptrs
  • allow user to either leave array as is or recalculate it if the array already exists

Naming Conventions

Naming of variables should descriptive where needed. Loop Control Variables can use i if warranted. Most of these conventions are enforced through the clang-tidy and clang-format configuration files. See the file simplnx/docs/Code_Style_Guide.md for a more in depth explanation.

Filter Checklist

The help file simplnx/docs/Porting_Filters.md has documentation to help you port or write new filters. At the top is a nice checklist of items that should be noted when porting a filter.

Unit Testing

The idea of unit testing is to test the filter for proper execution and error handling. How many variations on a unit test each filter needs is entirely dependent on what the filter is doing. Generally, the variations can fall into a few categories:

  • 1 Unit test to test output from the filter against known exemplar set of data
  • 1 Unit test to test invalid input code paths that are specific to a filter. Don't test that a DataPath does not exist since that test is already performed as part of the SelectDataArrayAction.

Code Cleanup

  • No commented out code (rare exceptions to this is allowed..)
  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added example pipelines that use the filter
  • Classes and methods are properly documented

@jmarquisbq jmarquisbq requested a review from imikejackson June 13, 2024 19:29
@jmarquisbq jmarquisbq self-assigned this Jun 13, 2024
@jmarquisbq jmarquisbq added bug Something isn't working enhancement New feature or request labels Jun 13, 2024
@jmarquisbq jmarquisbq force-pushed the fix/NodeBasedGeomListGeneration branch from 2085de1 to d758ef2 Compare June 14, 2024 12:46
@jmarquisbq jmarquisbq requested a review from JDuffeyBQ June 14, 2024 15:16
Copy link
Contributor

@imikejackson imikejackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was not part of the PR, but I would like see the error messages spruced up with as much information as possible. In some cases we could include some of the values that caused the failure?

@imikejackson imikejackson force-pushed the fix/NodeBasedGeomListGeneration branch from d758ef2 to ec1ad05 Compare June 20, 2024 15:22
@imikejackson
Copy link
Contributor

Waiting on #998

@imikejackson
Copy link
Contributor

#984 needs this PR

@imikejackson imikejackson force-pushed the fix/NodeBasedGeomListGeneration branch from ec1ad05 to 3f757b4 Compare June 20, 2024 20:23
@imikejackson imikejackson self-requested a review June 20, 2024 20:24
@imikejackson imikejackson enabled auto-merge (squash) June 20, 2024 20:26
- properly check for nullptrs
- allow user to either leave array as is or recalculate it if the array already exists
@imikejackson imikejackson force-pushed the fix/NodeBasedGeomListGeneration branch from 3f757b4 to 96a75e6 Compare June 21, 2024 00:27
@imikejackson imikejackson merged commit baf9702 into BlueQuartzSoftware:develop Jun 21, 2024
8 checks passed
imikejackson pushed a commit to imikejackson/simplnx that referenced this pull request Oct 20, 2024
…unctions (BlueQuartzSoftware#991)

- Fix bugs where the nullptr check was not performed in the proper order
- Update some of the algorithms to allow for the recalculation of the elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants